Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustdoc: fix intra-link for generic trait impls #92792

Merged
merged 2 commits into from
Jan 16, 2022

Conversation

mdibaiee
Copy link
Contributor

fixes #92662

r? @camelid

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 11, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 11, 2022
Comment on lines 907 to 916
|| match (impl_type.kind(), ty.kind()) {
(ty::Adt(def, _), ty::Adt(tydef, _)) => {
debug!("adt def_id: {:?}", def.did);
def.did == tydef.did
}
(ty::Foreign(def_id), ty::Foreign(tydef_id)) => def_id == tydef_id,
_ => false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camelid your intuition was right: this piece was necessary, but I was not sure at the time how to compare them, but I think this is the way: if they both having matching types, we compare the def_id alone, this allows users to write Foo without the generic type parameters, and still have it match with Foo<T>. That's my understanding.

Regarding the Foreign, I'm not sure what that means to be honest, any ideas how can I test that case too to avoid regressions again?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that explanation sounds right.

The Foreign is an extern type I believe (feature gate #![feature(extern_types)]; issue #43467).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camelid I actually realised there are already tests for Foreign, and because the ty::Foreign only has one field, the def_id, doing a == covers what we are doing. The tests are passing without this case in the match. I removed them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That test doesn't test trait impls on extern types, though. Can you please add a test case for that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true that TyKind has derived Eq, but Ty manually implements it and uses pointer equality (because Tys are interned).

What I was asking is why it's okay to skip manually equating Foreign but not Adt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camelid ah I see. I'm not sure then... I suppose we have the same question about all the other types, not just Foreign

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh 🤦

The reason it's necessary for Adts is because we want to ignore generics since the ty we're matching against may just have placeholders (or similar). Can you add a comment about that?

Copy link
Contributor Author

@mdibaiee mdibaiee Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes... that's what I meant with my initial comment on this piece #92792 (comment), I didn't get my message through well 😁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think I read your message and understood partially but then got confused later 😆

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2022
@mdibaiee mdibaiee force-pushed the 92662/fix-intra-doc-generics branch 2 times, most recently from a647025 to 4a94f5f Compare January 13, 2022 11:23
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@camelid
Copy link
Member

camelid commented Jan 13, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jan 13, 2022

📌 Commit ae20500 has been approved by camelid

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 13, 2022
@mdibaiee
Copy link
Contributor Author

@camelid thanks for your review!

@camelid camelid added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jan 13, 2022
@camelid
Copy link
Member

camelid commented Jan 13, 2022

Beta-nominating because I believe the regression is on beta now.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022
…cs, r=camelid

rustdoc: fix intra-link for generic trait impls

fixes rust-lang#92662

r? `@camelid`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022
…cs, r=camelid

rustdoc: fix intra-link for generic trait impls

fixes rust-lang#92662

r? ``@camelid``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022
…cs, r=camelid

rustdoc: fix intra-link for generic trait impls

fixes rust-lang#92662

r? ```@camelid```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022
…cs, r=camelid

rustdoc: fix intra-link for generic trait impls

fixes rust-lang#92662

r? ````@camelid````
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#92487 (Fix unclosed boxes in pretty printing of TraitAlias)
 - rust-lang#92581 (ARMv6K Horizon - Enable default libraries)
 - rust-lang#92619 (Add diagnostic items for macros)
 - rust-lang#92635 (rustdoc: Yet more intra-doc links cleanup)
 - rust-lang#92646 (feat: rustc_pass_by_value lint attribute)
 - rust-lang#92706 (Clarify explicitly that BTree{Map,Set} are ordered.)
 - rust-lang#92710 (Include Projections when elaborating TypeOutlives)
 - rust-lang#92746 (Parse `Ty?` as `Option<Ty>` and provide structured suggestion)
 - rust-lang#92792 (rustdoc: fix intra-link for generic trait impls)
 - rust-lang#92814 (remove unused FIXME)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 682ad02 into rust-lang:master Jan 16, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 16, 2022
@mdibaiee mdibaiee deleted the 92662/fix-intra-doc-generics branch January 16, 2022 20:35
@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 20, 2022
@pnkfelix
Copy link
Member

discussed in compiler triage meeting. backport approved.

@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 20, 2022
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.60.0, 1.59.0 Jan 20, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2022
…r=Mark-Simulacrum

[beta] Fix CVE-2022-21658

See https://blog.rust-lang.org/2022/01/20/cve-2022-21658.html. Patches reviewed by `@m-ou-se.`

Also backports:

*  resolve rustfmt issue with generated files rust-lang#92912
*  rustdoc: fix intra-link for generic trait impls rust-lang#92792
*  Fix rust logo style rust-lang#92764

r? `@ghost`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rustdoc: regression in intra-doc link to trait function
8 participants